New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve @babel/runtime
esm stability
#12883
Conversation
The `useESModules` option is unnecessary only when the target bundlers support exports conditions in `package.json`. When they don't, it's not possible to force them to use the native ESM files unless you use this option, that makes it work by using a different non-overloaded entrypoint.
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/42444/ |
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 03208b5:
|
If we don't get any emergent bug reports, can you add runtime consumer e2e test in this PR? So we are confident that it does not break some bundler/engine versions. |
Yes |
ff0fd42
to
a3ee405
Compare
5a6c1d8
to
1fbcb23
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with comments. Although we haven't received any bug reports on Rollup, should we add Rollup integration test?
@@ -0,0 +1,3 @@ | |||
import "./import-auto.mjs"; | |||
import "./import-esm.mjs"; | |||
import "./require-auto.cjs"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a case on ./require-esm.cjs
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should it work or throw?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should throw on Node. But other bundlers, especially legacy versions may have different behaviour.
"private": true, | ||
"exports": "./index.js", | ||
"devDependencies": { | ||
"@babel/runtime": "workspace:*", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you duplicate cases to @babel/runtime-corejs3
? We have extra core-js imports in that package.
Do we also need to test other bundlers, or do we trust that modern bundlers will do the correct thing? |
This makes sure that even if the target bundler doesn't support export conditions in `package.json`, an ESM helper will only load other ESM helpers.
e7a61df
to
238c29c
Compare
238c29c
to
03208b5
Compare
We didn't get any bug report about this because technically it works, but this PR introduces two improvements that make
@babel/runtime
better when using ESM helpers with older tools like Webpack 4:useESModules
. This option is still necessary if you want to force Webpack 4 to use esm helperspackage.json
(we don't need to rewrite imports to cjs polyfills, since they are cjs anyway).I also added a CI step to check that
@babel/runtime
works with different Node.js versions and bundlers.